Fix bug that would prevent uploaded images from being converted to the intended output format when having fallback formats enabled #1635
Conversation
mukeshpanchal27
added
[Type] Bug
|
There are currently 5 unit tests which are failing on trunk: There were 5 failures:The issue is in the core, it change the metadata for |
mukeshpanchal27
requested review from
adamsilverstein,
felixarntz and
westonruter
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@adamsilverstein Have you had chance to review the failed unit tests for trunk? Is it expected behaviour for trunk? Have you review WordPress/wordpress-develop#7733? |
There was a problem hiding this comment.
@mukeshpanchal27 I'm a bit concerned about simply removing the assertions. Why is that the right choice?
To me, these assertions failing means that something may be broken by the Core changes, that we either have to fix in the plugin or in Core. Or can you clarify why the expected behavior should have changed now?
|
On another note: It looks like the following tests are doing exactly the same:
One of them should probably be removed? |
|
@mukeshpanchal27 @adamsilverstein I found the actual problem here: It's a bug that's been in the plugin for a long time, but so far would only have affected images above the "big image" threshold. We just don't have any tests with a large image, so we should definitely add one with an image above that threshold to have that covered in the future. Here's the problem:
function webp_uploads_create_sources_property( array $metadata, int $attachment_id ): array {
$file = get_attached_file( $attachment_id, true );
// File does not exist.
if ( false === $file || ! file_exists( $file ) ) {
return $metadata;
}
/*
* We need to get the MIME type ideally from the file, as WordPress Core may have already altered it.
* The post MIME type is typically not updated during that process.
*/
$filetype = wp_check_filetype( $file );
if ( $filetype['type'] ) {
$mime_type = $filetype['type'];
} else {
$mime_type = get_post_mime_type( $attachment_id );
}
$valid_mime_transforms = webp_uploads_get_upload_image_mime_transforms();
// Not a supported mime type to create the sources property.
if ( ! is_string( $mime_type ) || ! isset( $valid_mime_transforms[ $mime_type ] ) ) {
return $metadata;
}
// Make sure the top level `sources` key is a valid array.
if ( ! isset( $metadata['sources'] ) || ! is_array( $metadata['sources'] ) ) {
$metadata['sources'] = array();
}
// Remaining code can probably stay as is.
}Additionally to fixing that, there's a few more places in the plugin where we call Last but not least, as mentioned let's add test coverage using an image that is above the "big image" threshold. The easiest way is probably to keep using the existing tests, but expand the |
Or just use the |
|
It always returns the MIME type of the uploaded image, but here we need the MIME type that the image is currently in, which may or may not be the output MIME type. The issue is that Core may already have processed the image before this function in our plugin is called, and at that point |
Yea, this is a limitation of the image metadata: it doesn't store the actual mime type of the uploaded image if it was converted. One workaround to speed this up (that I think is still used in core) is to look at the first item from the |
There was a problem hiding this comment.
@mukeshpanchal27 Thanks for the update, this fixes the test failures. Just two non-blocking notes.
Additionally, please consider my note about other usages of get_post_mime_type(). I just checked, and the one in webp_uploads_wrap_image_in_picture() looks incorrect to me as well, for the same reason here. Maybe we could take the logic you have here in lines 65-70 and move them to their own helper function? That way we could use that in both places. This could be done in a separate PR though, as it's not causing test failures today.
Co-authored-by: Weston Ruter <[email protected]>
That's the plan once these PR merged. |
felixarntz
changed the title
felixarntz
removed
the
skip changelog
Summary
Fixes #1634